feat(vault): econ metrics notifiers #5260

turadg · 2022-04-29T22:47:24Z

closes: #4649

Description

Per Contract (Director object)

~~Governance parameters, contract terms, fees~~ governed
Allowed collateral types
Current reward pool (fees collected so far)
~~Number and amount of shortfall~~ (DEFERRED Expose econ metrics on vault liquidations #5300)

Per Collateral (Manager object)

~~Governance parameters, including interest rate, liquidation ratio, liquidation penalty per collateral, current debt limit~~ governed
~~Current debt limit~~ governed
Number of vaults
Total outstanding debt (vaultKit publicNotifiers.asset)
Total collateral held
Compounded interest (vaultKit publicNotifiers.asset)
~~Liquidation strategy~~ governed
~~Liquidation state~~ (DEFERRED Expose econ metrics on vault liquidations #5300)

Per Vault

Debt (debtSnapshot.debt)
Collateral (locked)
Interest snapshot (debtSnapshot.interest)
Status (open, closed, liquidating, liquidated) vaultState

Use existing notifiers except for values that have high change frequency.

Security Considerations

Everything being exposed here is intentional.

Documentation Considerations

We will need to update these docs. Waiting on technical writer.

Testing Considerations

New tests for notifier output.

turadg · 2022-04-29T22:50:43Z

packages/run-protocol/src/vaultFactory/vaultDirector.js

@@ -33,6 +33,13 @@ import {
 const { details: X } = assert;

 /**
+ * @typedef {{
+ * governedValues: {}, // ??? what type? ??? aren't there already subscriptions?


Since governance is already on-chain, do we need to republish all the values or can we point to them somehow?

Also, @Chris-Hibbert WDYT of ParamManager having methods to support this?

It absolutely makes sense for ParamManager to provide support. I'm waiting on more detailed requirements.

I'm hoping to hear

what publishing/sharing mechanisms are needed

whether we want to emit events or statistics. At ${JOB-1} the s/w mostly said "x happened" and the monitoring systems did all the counting, averaging, etc. I can imagine we want more to happen in the contracts, so we'd emit counts, averages, maximums, etc.

turadg · 2022-04-29T22:52:53Z

packages/run-protocol/src/vaultFactory/vaultDirector.js

@@ -72,13 +80,17 @@ const initState = (zcf, directorParamManager, debtMint) => {

  const vaultParamManagers = makeScalarMap('brand');

+  // XXX need a way to register this data stream for consumption
+  const { updater: econMetrics } = makeNotifierKit();


@gibson042 I punted on mocking this because I wasn't sure where to put it. seems like @dtribble may have some designs in mind so I don't want to proceed too far.

dtribble

Generally great, but some namings and renamings are problematic. Note that those could mostly be not entangled with the core of this change.

dtribble · 2022-05-08T18:11:17Z

packages/run-protocol/src/vaultFactory/types.js

@@ -37,7 +38,7 @@
 * @param {Issuer} collateralIssuer
 * @param {Keyword} collateralKeyword
 * @param {VaultManagerParamValues} params
- * @returns {Promise<VaultManager>}
+ * @returns {Promise<VaultManagerObject>}


"Object" is a terrible thing to stick in a name. It's completely vacuous.

Agreed. VaultManager was used already by ReturnType<typeof makeVaultManagerKit>['manager'] so I was trying to make a small change but I'll make the better change.

dtribble · 2022-05-08T18:12:15Z

packages/run-protocol/src/vaultFactory/prioritizedVaults.js

@@ -174,6 +174,7 @@ export const makePrioritizedVaults = (reschedulePriceCheck = () => {}) => {
    addVault,
    entries: vaults.entries,
    entriesPrioritizedGTE,
+    getSize: vaults.getSize,


"count" would be much better "size". "Size" could mean any number of things (total market cap? AUM? disk space used?)

dtribble · 2022-05-08T18:14:16Z

packages/run-protocol/src/vaultFactory/vault.js

@@ -66,14 +66,15 @@ const validTransitions = {
 /**
 * @typedef {Phase[keyof typeof Phase]} TitlePhase
 *
- * @typedef {object} VaultUIState
+ * @typedef {object} VaultTitleState


"Title" has the wrong connotations:

it's a thing looked up in a registry

it feels ACL-based

it's primarily the meta-data, not the authority.

Thus the "title" woudl be more the invitation made in a transfer request (I sign over the title to you and then you get the keys to the property)

This is not just being pedantic. Using a term "near but wrong" would be a larger cause of deep confusion than using a new or clumsy term. Hence, "outer" would still be better.

ref

outer

handle (but used elsewhere ina. consistent way without operations)

proxy (but used in JS already)

front

facet

facade (implies fake though)

aspect (but "aspect oriented programming polluted that)

This is not just being pedantic. Using a term "near but wrong" would be a larger cause of deep confusion than using a new or clumsy term.

Agree naming is important and "near but wrong' can be worse that obviously wrong.

Thus the "title" woudl be more the invitation made in a transfer request

I don't agree with the bullets supporting it but I agree with this conclusion.

Note that this just a typename. In the vault offer result it's,

publicNotifiers: { vault: title.getNotifier(), asset: assetNotifier, },

So how about VaultNotifierState? VaultState could also work but given how easy it is to change type names I'd err towards more explicit and if we find over time the disambiguation is unnecessary we can shorten it.

EDIT: I went with VaultNotification.

dtribble · 2022-05-08T18:15:42Z

packages/run-protocol/src/vaultFactory/vault.js

@@ -66,14 +66,15 @@ const validTransitions = {
 /**
 * @typedef {Phase[keyof typeof Phase]} TitlePhase
 *
- * @typedef {object} VaultUIState


We need a general term for the state snapshot from a notifier. We could remove "UI" from that with "vaultSnapshot"? but "vaultState" isn't terrible.

Came up above and proposed Notification. I could see that being used for the wrapping object (with value and updateCount) but that's so generic as to not need a name.

dtribble · 2022-05-08T18:19:16Z

packages/run-protocol/src/vaultFactory/vaultDirector.js

@@ -254,6 +275,7 @@ const machineBehavior = {
      }
      // TODO add aggregate debt tracking at the vaultFactory level #4482
      // totalDebt = AmountMath.add(totalDebt, toMint);
+      facets.machine.notifyEcon();


what is "machine"?

That term started here 15322a1#diff-0b7d727a28b84089ac4ebe9b76f8957bb71bfee261640828bad01c4189cbe73fR283

I kept it because it distinguishes between the "creator facet" of the contract and the "limited creator facet" that it can return. I think we should move from "this thing is for what consumer" to "what this thing does or how". (For the same reasons that "public notifiers" is better than "UI notifiers")

dtribble · 2022-05-08T19:26:49Z

packages/run-protocol/src/vaultFactory/vaultManager.js

@@ -237,12 +255,23 @@ const helperBehavior = {
      compoundedInterest: state.compoundedInterest,
      interestRate,
      latestInterestUpdate: state.latestInterestUpdate,
-      totalDebt: state.totalDebt,
+      // XXX move to EconState and type as present with null
      liquidatorInstance: state.liquidatorInstance,


That should be in governance notifier I would think

dtribble · 2022-05-08T19:30:46Z

packages/run-protocol/test/vaultFactory/test-vaultFactory.js

-  trace(t, 'pa', { governorInstance, vaultFactory, lender, priceAuthority });
+  trace(t, 'pa', {
+    governorInstance,
+    vaultFactory: vaultFactoryCreator,


remove vaultFactory:

dtribble · 2022-05-08T19:31:30Z

packages/run-protocol/test/vaultFactory/test-vaultFactory.js

@@ -364,7 +368,11 @@ test('first', async t => {
    undefined,
    500n,
  );
-  const { vaultFactory, lender, aethVaultManager } = services.vaultFactory;
+  const {
+    vaultFactoryCreator: vaultFactory,


it would be good to not have this property renaming.

goal was to not have vaultFactory.vaultFactory as before. granted the type system indicates they're different, but just eyeballing it's not clear.

In general I think we would benefit from more consistency in how we name facets, contracts, and virtual/durable objects in contracts. That's a bigger endeavor that is out of scope so I'll revert this change.

dtribble · 2022-05-08T19:35:58Z

packages/run-protocol/test/vaultFactory/test-vaultFactory.js

  const [
    governorInstance,
-    vaultFactory,
+    vaultFactoryCreator,


this renaming causes a lot of churn. If you really want it, the uses below need to be updated as well so we dont' have a bunch of vaultFactory: vaultFactoryCreator. Given that the type system clarifies that it's the creator facet, this seems unnecessary.

dtribble · 2022-05-08T19:36:08Z

packages/run-protocol/test/vaultFactory/test-vaultFactory.js

@@ -784,7 +792,7 @@ test('vaultFactory display collateral', async t => {
    500n,
  );

-  const { vaultFactory } = services.vaultFactory;
+  const { vaultFactoryCreator: vaultFactory } = services.vaultFactory;


…fusion)

Chris-Hibbert · 2022-05-09T20:00:55Z

packages/run-protocol/src/vaultFactory/vaultManager.js

      liquidatorInstance: state.liquidatorInstance,
    });
    state.assetUpdater.updateState(payload);
  },

+  /** @param {MethodContext} context */
+  metricsNotify: ({ state }) => {


notify would conventionally be a fine name, but with notifier/updater the notifier half is about getting notified and the updater half is about sending notifications. I think this needs to be updateMetrics.

Chris-Hibbert · 2022-05-10T00:45:31Z

packages/run-protocol/src/vaultFactory/vaultManager.js

      liquidatorInstance: state.liquidatorInstance,
    });
    state.assetUpdater.updateState(payload);
  },

+  /** @param {MethodContext} context */
+  metricsNotify: ({ state }) => {


I think metricNotify should be called on liquidateAll(), but I don't see how it will be.

Do individual liquidations invoke it?

They should. 41fa373

though I ran into an accounting issue that isn't obvious to solve and will be changed by #5343 so I'll come back to this after that.

Chris-Hibbert · 2022-05-16T21:55:15Z

/home/runner/work/agoric-sdk/agoric-sdk/packages/run-protocol/test/vaultFactory/test-vaultFactory.js
Error: 43:1 error @agoric/notifier import should occur before import of ../../src/makeTracer.js import/order

confident I executed requested changes

…rics

samsiegart · 2022-06-02T15:37:38Z

packages/run-protocol/src/vaultFactory/vaultManager.js

@@ -34,11 +38,15 @@ const trace = makeTracer('VM', false);
 *  compoundedInterest: Ratio,
 *  interestRate: Ratio,
 *  latestInterestUpdate: bigint,
- *  totalDebt: Amount<'nat'>,


This caused a null reference error in the UI https://github.com/Agoric/dapp-treasury/blob/main/ui/src/components/vault/VaultConfigure/VaultConfigure.jsx#L200. I guess typescript/CI didn't catch this somehow...

Ah, I didn't realize that was being used. Yeah, the dapp-treasury repo currently has no type checking Agoric/dapp-treasury#97

turadg commented Apr 29, 2022

View reviewed changes

turadg force-pushed the 4649-expose-vaultFactor-econ-metrics branch 4 times, most recently from db362c6 to 74e672d Compare May 5, 2022 20:53

turadg marked this pull request as ready for review May 5, 2022 21:01

turadg requested a review from dtribble as a code owner May 5, 2022 21:01

turadg force-pushed the 4649-expose-vaultFactor-econ-metrics branch from 74e672d to 8761fe1 Compare May 5, 2022 21:26

dtribble previously requested changes May 8, 2022

View reviewed changes

turadg added 8 commits May 9, 2022 09:35

chore(vault): type VaultUIState -> VaultTitleState

9cedbdc

chore(zoe): type setupZCFTest

5334672

chore(vault): disambiguate VaultManager type (facet naming confusion)

af797d9

feat(vault): vaultDirector econ notifier

b4a5fcf

feat(vault): vaultManager econNotifier

696b651

feat(vault): number of vaults per manager in EconState

8fc22d7

style: fix 'object' type casing

4a0d2d6

fixup! chore(vault): type VaultUIState -> VaultTitleState

a6f5d8b

turadg changed the title ~~track vault factory econ metrics~~ feat(vault): econ metrics notifiers May 9, 2022

turadg added 3 commits May 9, 2022 09:52

'econ' -> 'metrics' (will squash)

ae03243

fixup! chore(vault): disambiguate VaultManager type (facet naming con…

d0c7612

…fusion)

size -> count

c5bd54d

turadg requested review from dtribble and Chris-Hibbert May 9, 2022 18:31

turadg force-pushed the 4649-expose-vaultFactor-econ-metrics branch from 8761fe1 to c5bd54d Compare May 9, 2022 18:31

turadg added 2 commits May 9, 2022 11:35

undo name change in test

e4401c1

lint

424776f

Chris-Hibbert reviewed May 10, 2022

View reviewed changes

turadg added 3 commits May 16, 2022 08:31

handleFooOffer style per #5179

d9c0607

chore(zoe): more typing offerTo and Invitation

024c205

plan

0cb7b4b

turadg added 14 commits May 16, 2022 08:31

fixup! rm obsolete penaltyPoolSeat

db9e406

consume reserve instance instead of facet

4c92859

typecheck liquidator,vaultFactory tests

fff80b0

fix test typo tsc caught

9e30183

fix invitation param and typecheck that would have caught it

d8fba65

update comment docs

74460d2

work around type resolution shortcomings

bbb4ea9

better comments

01c3fe6

Merge branch 'master' into 5167-liq-penalty

96c9811

Merge branch 'master' into 4649-expose-vaultFactor-econ-metrics

443a932

fixup undo vaultFactoryCreator

58f8d9d

getMetrics

3fe00a4

use subscriptions

30a8eaf

revert rename getNotifier (no ambiguity anymore)

9dd27d9

turadg requested a review from Chris-Hibbert May 16, 2022 21:34

turadg added the automerge:squash Automatically squash merge label May 16, 2022

turadg added 6 commits May 16, 2022 15:09

fix lint my editor didn't show 🤷

905e07c

fixup! getMetrics

d1b2344

update metrics after each liquidation

41fa373

Merge branch '5167-liq-penalty' into 4649-expose-vaultFactor-econ-met…

d3bc2d9

…rics

fix test for manager notifiers

fc54589

Merge branch 'master' into 4649-expose-vaultFactor-econ-metrics

cf8030d

Chris-Hibbert approved these changes May 18, 2022

View reviewed changes

Merge branch 'master' into 4649-expose-vaultFactor-econ-metrics

38219e8

mergify bot merged commit 6c3cdf3 into master May 18, 2022

mergify bot deleted the 4649-expose-vaultFactor-econ-metrics branch May 18, 2022 19:25

samsiegart reviewed Jun 2, 2022

View reviewed changes

turadg mentioned this pull request Jun 2, 2022

Implement type checking Agoric/dapp-treasury#97

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vault): econ metrics notifiers #5260

feat(vault): econ metrics notifiers #5260

turadg commented Apr 29, 2022 •

edited

Loading

turadg Apr 29, 2022

Chris-Hibbert May 2, 2022

turadg Apr 29, 2022

dtribble left a comment

dtribble May 8, 2022

turadg May 9, 2022

dtribble May 8, 2022

dtribble May 8, 2022

dtribble May 8, 2022

turadg May 9, 2022 •

edited

Loading

dtribble May 8, 2022

turadg May 9, 2022

dtribble May 8, 2022

turadg May 9, 2022 •

edited

Loading

dtribble May 8, 2022

dtribble May 8, 2022

dtribble May 8, 2022

turadg May 9, 2022

dtribble May 8, 2022

dtribble May 8, 2022

Chris-Hibbert May 9, 2022

turadg May 16, 2022

Chris-Hibbert May 10, 2022

turadg May 16, 2022

Chris-Hibbert commented May 16, 2022

samsiegart Jun 2, 2022

turadg Jun 2, 2022

feat(vault): econ metrics notifiers #5260

feat(vault): econ metrics notifiers #5260

Conversation

turadg commented Apr 29, 2022 • edited Loading

Description

Per Contract (Director object)

Per Collateral (Manager object)

Per Vault

Security Considerations

Documentation Considerations

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtribble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert commented May 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Apr 29, 2022 •

edited

Loading

turadg May 9, 2022 •

edited

Loading

turadg May 9, 2022 •

edited

Loading